feat: notifications - user storage controller #23353
Conversation
this allows developers to store synced and secure data that only clients can access this relies on the authentication controller
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… NOTIFY-415-user-storage-user-storage-controller
… NOTIFY-415-user-storage-user-storage-controller
other policies we can change, but the build-system policy is specifically for circleCI
… NOTIFY-415-user-storage-user-storage-controller
|
Fixing breaking builds & CI. Will mark ready for review one done |
… NOTIFY-415-user-storage-user-storage-controller
Prithpal-Sooriya
changed the title
Builds ready [2d3aec5]
Page Load Metrics (1546 ± 300 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
… NOTIFY-415-user-storage-user-storage-controller
this can be used for the notifications controller for trigger creation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #23353 +/- ##
===========================================
- Coverage 67.58% 67.42% -0.15%
===========================================
Files 1247 1251 +4
Lines 48934 49105 +171
Branches 12774 12790 +16
===========================================
+ Hits 33069 33109 +40
- Misses 15865 15996 +131 ☔ View full report in Codecov by Sentry. |
Builds ready [85ebd42]
Page Load Metrics (1218 ± 551 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| @@ -0,0 +1,117 @@ | |||
| import sjcl from 'sjcl'; | |||
There was a problem hiding this comment.
I think we might need a wider discussion on this.
Context
All MetaMask Clients (mobile, extension, portfolio) that want to use UserStorage must all use the same encryption/decryption mechanism.
Any data you store must be encrypted, and any data you retrieve must be decrypted.
This is what was approved on Portfolio - basic symmetric encryption using the storage key as the password.
Discussion
Can we also use this for extension? What are the risks? If there are other encryption mechanisms that we need, we will need it to be compatible for all metamask clients and also need to port Portfolio to use the same encryption mechanism.
There was a problem hiding this comment.
Is the storage key a symmetric key?
Also, we should rather use a KDF instead of a PBKDF if we are already working with cryptographic material.
There was a problem hiding this comment.
Yep the storage key is symmetric. And yep flexible to change to KDF.
TBH if there is a need to change to asymmetric encryption/decryption I think we can - I'll note this.
There was a problem hiding this comment.
I don't see a usecase for assymetric cryptography here. Care to elaborate?
There was a problem hiding this comment.
I agree - I don't see a need for asymmetric encryption - was open to it if it is recommended.
… NOTIFY-415-user-storage-user-storage-controller
we needed a way of disabling/enabling user storage/profile syncing
| * @returns the storage key | ||
| */ | ||
| async #createStorageKey(): Promise<string> { | ||
| const id = await this.#auth.getSessionIdentifier(); |
There was a problem hiding this comment.
It seems that the key used for encryption needs the session idenitfier. How is the session identifier deterministically generated across different installs?
There was a problem hiding this comment.
Great call out!
The session identifier here comes from our auth server. Under the hood this will be the "Auth V2 Profile Id", where we can sync multiple "logins" with the same profile.
|
Blocking this PR until the Security Threat Model Discussions have been concluded. |
… NOTIFY-415-user-storage-user-storage-controller
… NOTIFY-415-user-storage-user-storage-controller
… NOTIFY-415-user-storage-user-storage-controller
… NOTIFY-415-user-storage-user-storage-controller
… NOTIFY-415-user-storage-user-storage-controller
| * actions. | ||
| */ | ||
| #registerMessageHandlers(): void { | ||
| this.messagingSystem.registerActionHandler( |
There was a problem hiding this comment.
Didn't know we registered these handlers for the messaging system. Makes sense!
| @@ -152,6 +190,10 @@ export default class AuthenticationController extends BaseController< | |||
| return profile; | |||
| } | |||
|
|
|||
| public isSignedIn(): boolean { | |||
| return this.state.isSignedIn; | |||
There was a problem hiding this comment.
Dev Note - this is so state can be exposed through the messaging system. If we need to expose more state for other controllers we can widen this (or create new methods)
Builds ready [9e7ace6]
Page Load Metrics (883 ± 517 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
… NOTIFY-415-user-storage-user-storage-controller
Builds ready [509bfd4]
Page Load Metrics (1581 ± 730 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
I brought up one more thing during threat modeling where upon taking a bird's-eye view it doesn't seem to make sense to use profile id in the making of the key for encryption. |
Yep good comment! Lets sync and discuss this tomorrow - I think there is no hard requirement using the |
Prithpal-Sooriya
deleted the
NOTIFY-415-user-storage-user-storage-controller
branch
metamaskbot
added
the
release-11.16.0
Description
Followup PR on this: #23286
Allows developers to store synced and protected user information where only the user/client can read, write, and access.
This enables Profile Syncing and Notifications.
Related issues
Fixes: N/A
Manual testing steps
N/A, this controller is unconnected. It will be connected with other controllers in followup PRs.
Screenshots/Recordings
N/A
Before
N/A
After
N/A
Pre-merge author checklist
Pre-merge reviewer checklist